Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Support store Policy - SQL scheme #234

Merged
merged 6 commits into from
Apr 18, 2024

Conversation

kairoaraujo
Copy link
Collaborator

@kairoaraujo kairoaraujo commented Apr 10, 2024

What this PR does / why we need it

This is the first part of feature #158
This PR adds the schema in the ent to support the basic information to store Policy in the SQL Storage

Next:

Which issue(s) this PR fixes (optional)

Related to #158
Fixes #167 and #169

Acceptance Criteria Met

  • Docs changes if needed
  • Testing changes if needed
  • All workflow checks passing (automatically enforced)
  • All review conversations resolved (automatically enforced)
  • DCO Sign-off

Note for reviewers:

The review can focus on the three first commits, the last commit are db generate and migrations

Include the `go generate ./...` in the db migrations steps

`make db-migrations` now generates the ent files from schemes.

Signed-off-by: Kairo Araujo <[email protected]>
Adds the basic schema for the policy

Signed-off-by: Kairo Araujo <[email protected]>
)

// Attestation represents an attestation from a witness attestation collection
type AttestationPolicy struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy is a reserved name by ent

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.19%. Comparing base (a035c62) to head (77cfff9).
Report is 50 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #234       +/-   ##
===========================================
- Coverage   82.40%   68.19%   -14.22%     
===========================================
  Files          10       11        +1     
  Lines         358      503      +145     
===========================================
+ Hits          295      343       +48     
- Misses         43      105       +62     
- Partials       20       55       +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jkjell jkjell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! This is a great step towards full policy support. 🥳 I'll let @mikhailswift for the final approval. Just had a question or two from my ignorance of ent. 😅

ent/schema/attestationpolicy.go Show resolved Hide resolved
@@ -38,6 +38,7 @@ func (Statement) Fields() []ent.Field {
func (Statement) Edges() []ent.Edge {
return []ent.Edge{
edge.To("subjects", Subject.Type).Annotations(entgql.RelayConnection()),
edge.To("policies", AttestationPolicy.Type).Annotations(entgql.RelayConnection()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a question for someone who knows more about ent than I do but, if policies -> statements are a 1:1 mapping, do we need the RelayConnection()? Maybe related, should we call the edge the singular policy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense.
I will wait for @mikhailswift, so if I need to make more changes :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general way I think about whether an edge should be a RelayConnection is whether you want to query that edge directly or use it as an entry to the graph. We don't do so for the attestation_collections edge below, so I'd be inclined to keep it similar. Similarly to that, we probably want the policies edge to be Unique, since they are 1:1.

Copy link
Collaborator Author

@kairoaraujo kairoaraujo Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I have added the new commit 9b7918e

@kairoaraujo kairoaraujo changed the title Feature: Support store Policy Feature: Support store Policy - SQL scheme Apr 11, 2024
@kairoaraujo kairoaraujo modified the milestone: feat: Policy Apr 11, 2024
@kairoaraujo kairoaraujo force-pushed the policy-scheme branch 2 times, most recently from 04e6d30 to 44bcd98 Compare April 12, 2024 08:56
This commit includes the db generate for policy and the db migrations
`make db-migrations`

Signed-off-by: Kairo Araujo <[email protected]>
@kairoaraujo
Copy link
Collaborator Author

Hey @jkjell, @mikhailswift 👋
Let me know if you have further comments for this PR 😃
I plan to merge it tomorrow morning (CET) to unblock the next PR

@kairoaraujo kairoaraujo merged commit 84b08c0 into in-toto:main Apr 18, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

task: Add the schema for Policies
3 participants